-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run miner threads at priority SCHED_IDLE
(see sched(7)
)
#8928
base: master
Are you sure you want to change the base?
Conversation
8eb364f
to
d66cb2d
Compare
Why the change? Miners likely want mining to be at full-throttle, not at a lowered priority. There's also already the feature (built-in) to mine when idle (that's cross-platform). |
I'm aware of the feature, but I believe it's sub-optimal: The background mining feature sort of mimics running a thread at an idle priority, but by nature it's always a crude approximation.
So running a thread at an idle priority seems to me to be better in multiple aspects: It lets the system to utilize full CPU power available, lets the system react immediately to the CPU demand, and it's simpler to implement. If you prefer, I can add a new feature flag for this that'd complement the existing background mining feature. |
I don't disagree the current solution is sub-optimal, but it is 1) cross-platform and 2) surely impacts your changes anyway.
I think that's probably a better approach. Though a user can already simply ignore the |
src/cryptonote_basic/thread.h
Outdated
#ifdef _WIN32 | ||
|
||
// TODO: Add a Windows implementation. | ||
// Most likely by calling `SetThreadPriority` with | ||
// `THREAD_MODE_BACKGROUND_BEGIN` within the created thread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably needs to be something like:
auto thread = boost::thread(attrs, std::forward<Callable>(func));
auto handle = thread.native_handle();
SetThreadPriority(handle, THREAD_PRIORITY_LOWEST);
(THREAD_MODE_BACKGROUND_BEGIN only works for the current thread according to the docs).
I thought of this option too, but apparently that's problematic because it also prevents the main daemon from progressing as it should (https://monero.stackexchange.com/a/13983/16159). OK, I'll try to adapt it to all platforms and add an appropriate flag. |
5827c0a
to
11b34b2
Compare
This way miners can fully utilize all spare CPU power without affecting performance of the rest of the system.
Yes, I noticed that, and this was why I designed the function so that creates a thread, so that it can execute code within. PTAL? |
if (threads_idle) { | ||
m_threads.push_back(create_background_thread(m_attrs, boost::bind( | ||
&miner::worker_thread, this))); | ||
} else { | ||
m_threads.push_back(boost::thread(m_attrs, boost::bind( | ||
&miner::worker_thread, this))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor gripe, but formatting should follow the rest of the file (here, braces on newlines).
if (threads_idle) { | ||
MERROR("Feature --mining-threads-idle is an alternative to background mining and doesn't make sense to be used together."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, follow rest of file style
I can't speak to whether how you've now done it for Windows works or not (I presume you've tested), but just using THREAD_PRIORITY_LOWEST should suffice, and then launching in the same way as the non-windows version. I suppose if the way you've implemented works fine though, it may be better... |
Thank you for the review. Hmm, one test is failing, IDK why, I'll need to figure this out. (I'll be unavailable for some time so please be patient with me, I'll get back to this once I'm back.) |
Yes, tested, this approach seems to works quite nicely. According to the docs using
I'll fix the formatting and also I still need to figure out why one of the tests is broken. |
This way miners can fully utilize all spare CPU power without affecting performance of the rest of the system.